Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: include --nojekyll option flag #468

Closed
wants to merge 7 commits into from

Conversation

freitzzz
Copy link

No description provided.

@vamcs
Copy link

vamcs commented Jul 18, 2023

This flag would be really convenient. Although, I solved it by manually adding the .nojekyll file to the build folder by running touch dist/.nojekyll and adding the flag --dotfiles true to the command.

@freitzzz
Copy link
Author

The author doesn't seem to be maintaining the package anymore, what I did is I went ahead and created my own fork of the library and use it from there.

https://github.com/freitzzz/gh-pages

@w0rldart
Copy link

This would be a tremendous asset to this plugin. @tschaub could you please get this reviewed and merged?

@@ -113,6 +116,14 @@ exports.publish = function publish(basePath, config, callback) {
return;
}

if (options.cname) {
fs.writeFileSync(path.join(basePath, 'CNAME'), options.cname);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will result in the CNAME file being written to the dest directory (which may be a subdirectory not at the root of the repo). I think it should be written to the root instead.


if (options.nojekyll) {
fs.createFileSync(path.join(basePath, '.nojekyll'));
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same above as above about writing to the dest directory instead of the root. In addition, won't this file be ignored unless the --dotfiles option is also specified?

@tschaub tschaub closed this in #533 Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants